Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add range match policy type for enrich #65781

Closed
wants to merge 15 commits into from

Conversation

probakowski
Copy link
Contributor

This change adds new policy types to support all range types as enrich fields. It also adds new format field in enrich policy definition that lets you specify date format that will be used during creation of enrich index

Closes #59037

@probakowski probakowski added :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP v7.11.0 v8.0.0 labels Dec 2, 2020
@probakowski probakowski marked this pull request as ready for review December 2, 2020 23:21
@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Dec 2, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (Team:Core/Features)

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general this looks good and the change is smaller than I thought it would be.
Can you also add docs for this new enrich policy type?
Maybe also add a rest test to CommonEnrichRestTestCase?

@@ -67,6 +67,11 @@ public Processor create(Map<String, Processor.Factory> processorFactories, Strin
throw ConfigurationUtils.newConfigurationException(TYPE, tag, "max_matches", "should be between 1 and 128");
}

if (EnrichPolicy.RANGE_MATCH_TYPES.contains(policyType)) {
// range match uses the same processor as simple match
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe extend the comment here by including that a term query on a range field is translated into a range query with the values as lower and upper value. If multiple values are provided then each value is translates into a range query and that is then combined into a boolean query with should clauses, which means that on of the provided values need to fall into the range of the range field.

Copy link
Member

@jbaiera jbaiera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had just a few comments. +1 on adding some docs, but otherwise looks pretty good.


private static final ParseField QUERY = new ParseField("query");
private static final ParseField INDICES = new ParseField("indices");
private static final ParseField MATCH_FIELD = new ParseField("match_field");
private static final ParseField ENRICH_FIELDS = new ParseField("enrich_fields");
private static final ParseField FORMAT = new ParseField("elasticsearch_version");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should use a different field name in the constructor

@@ -245,13 +245,21 @@ private static void validateField(Map<?, ?> properties, String fieldName, boolea
}
}

private XContentBuilder resolveEnrichMapping(final EnrichPolicy policy) {
@SuppressWarnings("unchecked")
private XContentBuilder resolveEnrichMapping(final EnrichPolicy policy, GetIndexResponse getIndexResponse) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is getIndexResponse used in this method?

@jakelandis
Copy link
Contributor

@probakowski - can this be closed in favor of #76110 ?

@danhermann danhermann added v8.1.0 and removed v7.16.0 labels Oct 27, 2021
@mjmbischoff
Copy link
Contributor

@probakowski - can this be closed in favor of #76110 ?

The tests got reused, and I think all functionality is covered, please reopen if these assumptions are false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP Team:Data Management Meta label for data/management team v8.0.0-rc1 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ip_range in Enrichment
10 participants